Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add TypeScript typings definition file for v3 API #251

Merged

Conversation

notheotherben
Copy link
Contributor

@notheotherben notheotherben commented Jun 27, 2016

Hi guys,

Recently updated an application of ours to use your 3.0.0 library and noticed there weren't any up to date typings files in the wild. Seeing as the current approach with TypeScript is to embed these definition files in their respective NPM modules, I've taken the time to put together a compatible set of type definitions for your library.

In addition to adding the raw type definitions, I've also taken the liberty of adding a quick test that ensures the definition file is well formatted, it should help ensure that things aren't broken accidentally going forward.

As far as manual verification of the typings file goes, aside from running your unit test suites through the TypeScript compiler to confirm that they comply with the typings file (always a good way to ensure the types match what's expected), we're also running these typings on a production system with no issues as yet.

I'll ensure I sign the contributor license agreement and get that through to you guys, in the mean time if you wouldn't mind taking a look and letting me know if there's anything that stands out as being incorrect, or raising any issues you may have with this addition to the codebase.

Things that may be issues

  • There's now a devDependency on the TypeScript compiler. While it isn't significantly large, I notice you've tried to keep the number of dependencies you have to a minimum (build times I assume being the reason). We can easily remove it should you wish, however that will remove the ability to test that the definitions are indeed valid.
  • There's something else to manage as the API changes. Unfortunately that's not something one can do much about (aside from switching to TypeScript for the library, not really an option). What I've done previously with projects on which we hold a dependency is offer to assist in updating the definitions whenever the API does change. In exchange I just ask that any of the project leads try and keep me informed (using a CC) whenever they are aware of any API changes.

Kind regards and all the best,
Benjamin

@notheotherben
Copy link
Contributor Author

notheotherben commented Jun 27, 2016

Hey guys, just noticed that all the built in unit tests failed with a 401 error - suspecting that has something to do with your test suite environment variables as I haven't touched anything on that front - however if there's anything you'd like me to look at, please don't hesitate to ask.

@notheotherben
Copy link
Contributor Author

Hey guys, any word on whether this'll get included or whether I should instead be investigating other approaches to publishing it? Still having to hack our way around the lack of a usable type definition from any official source.

@thinkingserious
Copy link
Contributor

Hello @spartan563,

It's still on our backlog and we plan to merge it.

The only way this can move up our backlog queue is through +1's or comments on this thread.

Thanks for checkin in!

@notheotherben
Copy link
Contributor Author

I see, not a problem at all so long as there's a process in place - let me know if you need anything from me at any point and I'll do my best to help out.

@CelsoSantos
Copy link

CelsoSantos commented Aug 10, 2016

I too am writing an application in TypeScript and also need to use SendGrid. It's not that it's impossible to use without the Definition file, but it does become much harder as one has to constantly check on documentation instead of getting intellisense (in visual studio code) completions.

@thinkingserious
Copy link
Contributor

Thanks for the upvote @CelsoSantos, it helps move this issue up our queue.

@thinkingserious
Copy link
Contributor

Hi @spartan563,

Could you please resolve the conflicts so that I may test and merge? Thanks!

@notheotherben notheotherben force-pushed the feat/typescript-definitions branch from 596ea71 to 3b982fd Compare September 15, 2016 07:48
@notheotherben
Copy link
Contributor Author

Hi @thinkingserious, should be up to date, rebased on master.

@thinkingserious thinkingserious merged commit d4acbce into sendgrid:master Sep 15, 2016
@thinkingserious
Copy link
Contributor

@spartan563 please email us at [email protected] with your T-shirt size and mailing address :)

thinkingserious added a commit that referenced this pull request Sep 15, 2016
@frederickfogerty
Copy link

frederickfogerty commented Sep 19, 2016

Hey @spartan563 thanks for adding this to the lib, I LOVE it when modules have type definitions in their core. 🙌

I'm having a problem when trying to use this module in my TypeScript project. I'm receiving this error when exporting a SendGrid instance: Exported variable 'sendGrid' has or is using name 'SendGrid.SendGrid' from external module "[...]/node_modules/sendgrid/index" but cannot be named.

This is a repro:

import { SendGrid } from 'sendgrid';
import * as SG from 'sendgrid'; // This is what we usually do to try squash the error, it doesn't work in this scenario :(

const sendGrid = SendGrid(process.env.SENDGRID_API_KEY);
//    ^^^^^^^^ error here

export default sendGrid;

And since SendGrid is a namespace, I can't access it import it (?!), and thus can't explicitly type sendGrid with something like const sendGrid: SendGrid.SendGrid = ....

🙏 🙌 Any help would be greatly appreciated, don't make us fall back to any! 😱

@notheotherben
Copy link
Contributor Author

Hi @frederickfogerty, just spun up an essentially empty project and wasn't able to reproduce the issue you're describing.

I've put down the example code you gave me in a quick repo which demonstrates this, if you could please fork it and make any changes necessary on your side to reproduce the issue then I'd be more than willing to look into it further.

Repo: https://github.com/SPARTAN563/sendgrid-ts-demo

Regards,
Benjamin

@frederickfogerty
Copy link

Hey @spartan563, thanks for taking a look at my issue, I really appreciate it.

I have submitted a PR to your repo which reproduces the issue: notheotherben/sendgrid-ts-demo#1

Thanks!

notheotherben pushed a commit to notheotherben/sendgrid-nodejs that referenced this pull request Sep 20, 2016
See sendgrid#251 and <notheotherben/sendgrid-ts-demo#1> for more details on what wasn't working
@thinkingserious
Copy link
Contributor

@frederickfogerty,

Could the issue be that you are using sendGrid instead of SendGrid?

@frederickfogerty
Copy link

Hey @thinkingserious, thanks for looking at my issue.

By using SendGrid instead of sendGrid, I'm assuming you mean for the variable name? In that case, there would be duplicate (i.e. conflicting) identifiers and wouldn't compile.

i.e. this wouldn't compile

import { SendGrid } from 'sendgrid';
import * as SG from 'sendgrid'; // This is what we usually do to try squash the error, it doesn't work in this scenario :(

const SendGrid = SendGrid(process.env.SENDGRID_API_KEY);
//    ^^^^^^^^ error here

export default SendGrid;

shkabo pushed a commit to shkabo/sendgrid-nodejs that referenced this pull request Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants